Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Windows DLL load issues related to PyInstaller #377

Merged
merged 5 commits into from
Nov 9, 2024

Conversation

RobPasMue
Copy link
Member

Closes #376

@RobPasMue RobPasMue self-assigned this Nov 6, 2024
@RobPasMue
Copy link
Member Author

@phx-mkoninckx could you give it a try to the executable that gets generated from the workflow? We are resetting the DLL loading as suggested in the PyInstaller documentation now. Also took the opportunity to revert the hack we implemented for the paths.

@github-actions github-actions bot added the bug Something isn't working label Nov 6, 2024
@RobPasMue
Copy link
Member Author

(Linux failing build is expected and under investigation by @tusharbana-ansys)

@phx-mkoninckx
Copy link

phx-mkoninckx commented Nov 6, 2024

@RobPasMue

This does seem to resolve the specific issue I was looking at when I reported #376. However, I'm concerned about the revert of #344. The fix in #344 did actually have some impact; without it, PATH includes the _internal and _internal\PySide6 subdirectories from the directory where the Ansys Python Manager exists on disk:

(bug_repro) C:\Users\Admin>echo %PATH%
C:\Users\Admin\.ansys_python_venvs\bug_repro\Scripts;C:\Users\Admin\python-installer-qt-gui\dist\ansys_python_manager\_internal\PySide6;C:\Users\Admin\python-installer-qt-gui\dist\ansys_python_manager\_internal;C:\Program Files\Microsoft\jdk-11.0.18.10-hotspot\bin;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;C:\Windows\System32\OpenSSH\;C:\Program Files\Git\cmd;C:\Users\Admin\AppData\Local\Programs\Python\Python311\Scripts\;C:\Users\Admin\AppData\Local\Programs\Python\Python311\;C:\Users\Admin\AppData\Local\Microsoft\WindowsApps;

Overall, this is less concerning than the impact of calling SetDllDirectory, since:

  1. Generally, DLLs are located by searching the PATH as a last-ditch effort. This means that these alterations to the PATH are, practically, somewhat less likely to impact most applications, since most native Windows apps don't depend on finding their DLLs on the PATH and instead expect to find them in the same directory as the application. Or, if they do depend on the PATH to load certain DLLs, they are probably putting their expected locations on the PATH first before attempting such a load. (This is the case for ModelCenter -- the DOE plug-in is run out-of-process, and when ModelCenter Desktop launches the DOE plug-in, it adds a centralized installation directory for the oSL algorithms to the subprocess' PATH as the first item, so the presence of the _internal directory here winds up not mattering).
  2. When attempting to debug issues caused by alterations to the PATH, it's somewhat easier to tell what's going on and deal with it.

However, if #344 was really the fix for #343, then reverting it could cause #343 to regress?

@RobPasMue
Copy link
Member Author

Good point @phx-mkoninckx - I just checked it with the dev version and saw it was no longer in the PATH, buuut... that is not true if you used the full solution (executable on workflow). Reapplying the change. Thanks a lot for all the explanations @phx-mkoninckx!!

Copy link

@phx-mkoninckx phx-mkoninckx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks! @RobPasMue -- with respect to the changes to PATH, I start to wonder if what might be better than trying to undo the addition of the _internal directory would either be to:

a) attempt to directly read and use the registry key that Windows would use to set the environment of a newly-started command prompt for the PATH specifically -- i.e. what is set in the Control Panel for that environment variable
b) more generally look into ways to launch processes from Python such that they don't inherit the environment from the Python interpreter process but instead emulate the behavior of them being launched by the user from the Windows shell (i.e. explorer.exe; the initial environment is based on the Control Panel settings for environment variables). But that would be a separate PR / issue

@RobPasMue
Copy link
Member Author

Let's do that on a follow up PR - merging!

@RobPasMue RobPasMue merged commit 43cc64b into main Nov 9, 2024
23 of 24 checks passed
@RobPasMue RobPasMue deleted the fix/windows-dll-load branch November 9, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants